feat: TSR Device Feedback -> Rundown (SOFIE-311)#1731
Merged
Conversation
Julusian
added
the
Contribution from BBC
WalkthroughIntroduces end-to-end external-event reporting: peripheral devices send TSR events to Core, Core queues/merges studio jobs, publications compute per-device subscriptions from rundowns, playout jobs invoke blueprint onExternalEvent handlers, and the gateway wires device connections to subscription updates. ChangesExternal events end-to-end
Sequence Diagram(s)sequenceDiagram
participant TSRGateway as TSR Gateway
participant Core as Core Server
participant JobQueue as Job Queue Manager
participant Playout as Playout Worker
participant Blueprint as ShowStyle Blueprint
TSRGateway->>Core: reportExternalEvents(deviceId, events)
Core->>Core: validate device token & studioId
Core->>JobQueue: QueueOrUpdateStudioJob(OnExternalEvents, studioId, merge events)
JobQueue->>JobQueue: merge with tail job or enqueue new
JobQueue->>Playout: process OnExternalEvents job (studio)
Playout->>Playout: enumerate active playlists & rundowns
Playout->>Blueprint: onExternalEvent(context, persistentState, events[])
Blueprint->>Playout: return (state changes, notes)
Playout->>Playout: save persistent state & notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
5 tasks
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/playout-gateway/package.json (1)
58-58: Verify feature-branch nightly resolver is intentional before merging tomain.Line 58 pins a feature-branch nightly (
nightly-feat-tsr-device-feedback-20260429-145317-4079605bd.0). The dependency is used across 7 imports in playout-gateway (DevicesRegistry, MediaObject, DeviceOptionsAny, ActionExecutionResult, BaseRemoteDeviceIntegration, TSRDevicesManifestEntry, and AtemMediaPool types) and no conflicting TSR versions exist elsewhere in the repo. All imports use standard public API surface. If this PR lands onmain, plan to switch to anightly-mainor release build once available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/playout-gateway/package.json` at line 58, The dependency "timeline-state-resolver" is pinned to a feature-branch nightly ("10.0.0-nightly-feat-tsr-device-feedback-20260429-145317-4079605bd.0") used by imports DevicesRegistry, MediaObject, DeviceOptionsAny, ActionExecutionResult, BaseRemoteDeviceIntegration, TSRDevicesManifestEntry, and AtemMediaPool; before merging to main either replace that pinned feature-nightly with the intended stable/nightly-main or a release version (or add a clear TODO and justification) in package.json so main doesn't depend on a feature branch—update the version string accordingly and run install/build to confirm no type or API breaks across those seven import sites.packages/job-worker/src/ingest/generationRundown.ts (1)
302-304: Add a focused regression test for this new persisted field.This introduces new rundown persistence behavior; please add an ingest test asserting
externalEventSubscriptionsfrom blueprint output are stored and retrievable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/ingest/generationRundown.ts` around lines 302 - 304, Add a regression test that verifies the new persisted field externalEventSubscriptions produced by the blueprint is saved and can be retrieved: in the ingest test suite that exercises generationRundown (where translateUserEditsFromBlueprint(...) and externalEventSubscriptions are passed into the persistence call), create a blueprint output containing a non-empty externalEventSubscriptions array, run the existing ingestion path, then query the persisted rundown and assert the stored rundown.rundown.externalEventSubscriptions strictly equals the blueprint value; ensure the test covers both save and subsequent retrieval to catch regressions.meteor/server/worker/jobQueue.ts (1)
364-402: Add focused tests for merge-vs-enqueue behavior.Please add coverage for:
- merge into matching high-priority tail,
- enqueue when tail is different/null,
- no mutation of ordering across mixed job names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/worker/jobQueue.ts` around lines 364 - 402, Add unit tests for mergeOrQueueJob covering three scenarios: (1) when the last high-priority job has the same name, verify that mergeOrQueueJob calls generateData with the existing tail.spec.data, mutates tail.spec.data in-place, does not change queue.jobsHighPriority length, and logs the merge (use mergeOrQueueJob and inspect the queue returned by `#getOrCreateQueue` or access the queue object after invoking); (2) when the tail is absent or has a different name, verify a new JobEntry is pushed with id from getRandomString(), name equal to jobName and data from generateData(null), that queue.metricsTotal.inc() is called and `#notifyWorker` is invoked (spy/mock generateData, getRandomString, metricsTotal.inc, and `#notifyWorker`); (3) when multiple job names exist verify ordering is preserved (calls that merge should only affect the very last entry and must not reorder earlier entries) by setting up a queue.jobsHighPriority array with mixed named entries, calling mergeOrQueueJob and asserting only the tail changed or a new entry appended. Use spies/mocks for generateData and minimal assertions on queue.jobsHighPriority length, element identities, and tail.spec.data to ensure no unintended reordering or replacement.meteor/server/worker/worker.ts (1)
254-265: Align startup-test guard withQueueStudioJob.
QueueStudioJobblocks during startup tests (isInTestWrite()), but this new path does not. Keeping the same guard avoids accidental writes in test bootstrap flows.Suggested fix
export function QueueOrUpdateStudioJob<T extends keyof StudioJobFunc>( jobName: T, studioId: StudioId, generateData: (existing: Parameters<StudioJobFunc[T]>[0] | null) => Parameters<StudioJobFunc[T]>[0] ): void { + if (isInTestWrite()) throw new Meteor.Error(404, 'Should not be reachable during startup tests') if (!studioId) throw new Meteor.Error(500, 'Missing studioId') queueManager.mergeOrQueueJob(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/worker/worker.ts` around lines 254 - 265, Add the same startup-test guard used in QueueStudioJob to QueueOrUpdateStudioJob: at the top of QueueOrUpdateStudioJob check isInTestWrite() and return early (no-op) when true to avoid test-time writes; ensure the guard runs before any throws or calls to queueManager.mergeOrQueueJob and keep the existing studioId null check and mergeOrQueueJob call intact (function names: QueueOrUpdateStudioJob, QueueStudioJob, isInTestWrite, queueManager.mergeOrQueueJob).packages/job-worker/src/playout/externalEvents.ts (1)
109-113: Documented type cast acknowledged; consider runtime guard.The comment explains that
PeripheralDeviceExternalTSREventuses a loosedeviceType: stringto accommodate custom TSR plugins, whileBlueprintExternalTSREventuses the strongly-typed TSR enum. The cast throughunknownis intentional but bypasses compile-time type checking.If a future event source (non-TSR) is added, this blanket cast could silently allow mismatched shapes into the blueprint. Consider adding a lightweight runtime assertion (e.g., verifying
event.type === 'tsr') to fail fast on unexpected event types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/externalEvents.ts` around lines 109 - 113, The blanket cast from wireEvents to BlueprintExternalEvent[] (wireEvents -> blueprintEvents) can hide mismatched shapes; add a lightweight runtime guard before or during the cast that asserts each event is a TSR event (e.g., verify event.type === 'tsr' for all entries in wireEvents) and either filter out non-TSR events or throw a clear error; update the code around the blueprintEvents assignment (the wireEvents variable and the resulting blueprintEvents) to perform this check and fail fast if an unexpected event type is encountered, while keeping the documented cast for deviceType handling (PeripheralDeviceExternalTSREvent -> BlueprintExternalEvent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@meteor/server/api/peripheralDevice.ts`:
- Around line 316-337: reportExternalEvents currently appends unbounded incoming
events into the pending StudioJobs.OnExternalEvents job, which can grow without
limit under bursts; update the reportExternalEvents handler to validate each
incoming PeripheralDeviceExternalEvent (type/required fields) and enforce a hard
maximum batch size before merging (e.g. define a MAX_EVENTS_PER_JOB constant),
then when calling QueueOrUpdateStudioJob merge sanitized events and truncate the
resulting array to that maximum (choose a policy: drop oldest or newest) so the
stored job data cannot grow unbounded; reference the reportExternalEvents
function, StudioJobs.OnExternalEvents and QueueOrUpdateStudioJob when making the
change.
In `@meteor/server/publications/externalEventSubscriptions.ts`:
- Around line 163-166: Add runtime validation for the `type` parameter in the
same function that currently does `check(deviceId, String)` (in
meteor/server/publications/externalEventSubscriptions.ts) by calling
`check(type, String)` before using `type` to filter subscription documents and
construct the observer key (e.g., where `peripheralDevice` is obtained via
`checkAccessAndGetPeripheralDevice`); this ensures `type` is a string and
prevents invalid values from affecting the publication observer behavior.
In `@packages/job-worker/src/playout/externalEvents.ts`:
- Around line 118-124: The category string passed into
storeNotificationsForCategory currently uses getRandomId()
("externalEvent:${getRandomId()}"), causing a new category on every call and
unbounded notification growth; change the category to a deterministic identifier
(for example "externalEvent:${playlist.playlistId}" or
"externalEvent:${playlist.rundownId}" or include blueprint.blueprintId) so
repeated external-event calls overwrite the same category. Locate the call to
storeNotificationsForCategory in externalEvents.ts and replace the getRandomId()
suffix with a stable field from the playlist or rundown (using
playlist.playlistId, playlist.rundownId, or blueprint.blueprintId) so
notifications are replaced instead of accumulated.
In `@packages/playout-gateway/src/tsrHandler.ts`:
- Around line 873-910: The _updateEventSubscriptions method only runs on DB
collection changes, so add listeners to the TSR connection manager to refresh
subscriptions whenever connection topology changes: in the class initialization
(where tsr is available) subscribe to the connection manager's
connection-added/connection-removed/connection-updated (or equivalent) events
and call this._updateEventSubscriptions() when those fire (also handle cases
where a connection's deviceId changes); ensure you remove those listeners on
teardown. Reference the existing _updateEventSubscriptions method and
tsr.connectionManager.getConnections()/container.device to locate where to wire
the event handlers.
---
Nitpick comments:
In `@meteor/server/worker/jobQueue.ts`:
- Around line 364-402: Add unit tests for mergeOrQueueJob covering three
scenarios: (1) when the last high-priority job has the same name, verify that
mergeOrQueueJob calls generateData with the existing tail.spec.data, mutates
tail.spec.data in-place, does not change queue.jobsHighPriority length, and logs
the merge (use mergeOrQueueJob and inspect the queue returned by
`#getOrCreateQueue` or access the queue object after invoking); (2) when the tail
is absent or has a different name, verify a new JobEntry is pushed with id from
getRandomString(), name equal to jobName and data from generateData(null), that
queue.metricsTotal.inc() is called and `#notifyWorker` is invoked (spy/mock
generateData, getRandomString, metricsTotal.inc, and `#notifyWorker`); (3) when
multiple job names exist verify ordering is preserved (calls that merge should
only affect the very last entry and must not reorder earlier entries) by setting
up a queue.jobsHighPriority array with mixed named entries, calling
mergeOrQueueJob and asserting only the tail changed or a new entry appended. Use
spies/mocks for generateData and minimal assertions on queue.jobsHighPriority
length, element identities, and tail.spec.data to ensure no unintended
reordering or replacement.
In `@meteor/server/worker/worker.ts`:
- Around line 254-265: Add the same startup-test guard used in QueueStudioJob to
QueueOrUpdateStudioJob: at the top of QueueOrUpdateStudioJob check
isInTestWrite() and return early (no-op) when true to avoid test-time writes;
ensure the guard runs before any throws or calls to queueManager.mergeOrQueueJob
and keep the existing studioId null check and mergeOrQueueJob call intact
(function names: QueueOrUpdateStudioJob, QueueStudioJob, isInTestWrite,
queueManager.mergeOrQueueJob).
In `@packages/job-worker/src/ingest/generationRundown.ts`:
- Around line 302-304: Add a regression test that verifies the new persisted
field externalEventSubscriptions produced by the blueprint is saved and can be
retrieved: in the ingest test suite that exercises generationRundown (where
translateUserEditsFromBlueprint(...) and externalEventSubscriptions are passed
into the persistence call), create a blueprint output containing a non-empty
externalEventSubscriptions array, run the existing ingestion path, then query
the persisted rundown and assert the stored
rundown.rundown.externalEventSubscriptions strictly equals the blueprint value;
ensure the test covers both save and subsequent retrieval to catch regressions.
In `@packages/job-worker/src/playout/externalEvents.ts`:
- Around line 109-113: The blanket cast from wireEvents to
BlueprintExternalEvent[] (wireEvents -> blueprintEvents) can hide mismatched
shapes; add a lightweight runtime guard before or during the cast that asserts
each event is a TSR event (e.g., verify event.type === 'tsr' for all entries in
wireEvents) and either filter out non-TSR events or throw a clear error; update
the code around the blueprintEvents assignment (the wireEvents variable and the
resulting blueprintEvents) to perform this check and fail fast if an unexpected
event type is encountered, while keeping the documented cast for deviceType
handling (PeripheralDeviceExternalTSREvent -> BlueprintExternalEvent).
In `@packages/playout-gateway/package.json`:
- Line 58: The dependency "timeline-state-resolver" is pinned to a
feature-branch nightly
("10.0.0-nightly-feat-tsr-device-feedback-20260429-145317-4079605bd.0") used by
imports DevicesRegistry, MediaObject, DeviceOptionsAny, ActionExecutionResult,
BaseRemoteDeviceIntegration, TSRDevicesManifestEntry, and AtemMediaPool; before
merging to main either replace that pinned feature-nightly with the intended
stable/nightly-main or a release version (or add a clear TODO and justification)
in package.json so main doesn't depend on a feature branch—update the version
string accordingly and run install/build to confirm no type or API breaks across
those seven import sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b94813a-bfd3-48dc-b4e5-06cef5485324
⛔ Files ignored due to path filters (2)
meteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpackages/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
meteor/server/api/peripheralDevice.tsmeteor/server/publications/_publications.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/publications/rundown.tsmeteor/server/worker/jobQueue.tsmeteor/server/worker/worker.tspackages/blueprints-integration/src/api/showStyle.tspackages/blueprints-integration/src/context/adlibActionContext.tspackages/blueprints-integration/src/context/externalEventContext.tspackages/blueprints-integration/src/context/index.tspackages/blueprints-integration/src/context/playoutActionContext.tspackages/blueprints-integration/src/externalEvent.tspackages/blueprints-integration/src/index.tspackages/corelib/src/dataModel/Rundown.tspackages/corelib/src/worker/studio.tspackages/job-worker/src/ingest/generationRundown.tspackages/job-worker/src/ingest/model/IngestModel.tspackages/job-worker/src/ingest/model/implementation/IngestModelImpl.tspackages/job-worker/src/playout/adlibAction.tspackages/job-worker/src/playout/externalEvents.tspackages/job-worker/src/workers/studio/jobs.tspackages/playout-gateway/package.jsonpackages/playout-gateway/src/coreHandler.tspackages/playout-gateway/src/tsrHandler.tspackages/shared-lib/package.jsonpackages/shared-lib/src/peripheralDevice/externalEvents.tspackages/shared-lib/src/peripheralDevice/methodsAPI.tspackages/shared-lib/src/pubsub/peripheralDevice.ts
nytamin
approved these changes
May 14, 2026
Julusian
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared-lib/src/pubsub/peripheralDevice.ts (1)
140-144: 💤 Low valueParameter ordering diverges from the other subscription signatures.
Every other entry in
PeripheralDevicePubSubTypesleads withdeviceId(e.g.mountedTriggersForDevice,packageManagerExpectedPackages). This new entry placestypebeforedeviceId. It works (the gateway calls it as('tsr', deviceId)), but the inconsistency is easy to trip over. Consider leading withdeviceIdfor consistency.♻️ Proposed reordering (requires updating callers)
[PeripheralDevicePubSub.externalEventSubscriptionsForDevice]: ( - type: PeripheralDeviceExternalEvent['type'], deviceId: PeripheralDeviceId, + type: PeripheralDeviceExternalEvent['type'], token?: string ) => PeripheralDevicePubSubCollectionsNames.externalEventSubscriptionsCaller in
packages/playout-gateway/src/coreHandler.tswould change toautoSubscribe(..., this.core.deviceId, 'tsr').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared-lib/src/pubsub/peripheralDevice.ts` around lines 140 - 144, The new subscription signature PeripheralDevicePubSub.externalEventSubscriptionsForDevice places the event type parameter before deviceId, diverging from other subscriptions that lead with deviceId; change the signature to (deviceId: PeripheralDeviceId, type: PeripheralDeviceExternalEvent['type'], token?: string) => PeripheralDevicePubSubCollectionsNames.externalEventSubscriptions and update all callers (e.g., the autoSubscribe invocation in coreHandler.ts) to pass this.core.deviceId first and the event type second so the parameter order matches mountedTriggersForDevice and packageManagerExpectedPackages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/shared-lib/src/pubsub/peripheralDevice.ts`:
- Around line 140-144: The new subscription signature
PeripheralDevicePubSub.externalEventSubscriptionsForDevice places the event type
parameter before deviceId, diverging from other subscriptions that lead with
deviceId; change the signature to (deviceId: PeripheralDeviceId, type:
PeripheralDeviceExternalEvent['type'], token?: string) =>
PeripheralDevicePubSubCollectionsNames.externalEventSubscriptions and update all
callers (e.g., the autoSubscribe invocation in coreHandler.ts) to pass
this.core.deviceId first and the event type second so the parameter order
matches mountedTriggersForDevice and packageManagerExpectedPackages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a41ded80-0ece-4244-98f0-32676d414ca0
⛔ Files ignored due to path filters (1)
meteor/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (14)
meteor/server/api/peripheralDevice.tsmeteor/server/publications/externalEventSubscriptions.tsmeteor/server/worker/__tests__/jobQueue.test.tsmeteor/server/worker/worker.tspackages/blueprints-integration/src/api/showStyle.tspackages/blueprints-integration/src/context/index.tspackages/corelib/src/worker/studio.tspackages/job-worker/src/workers/studio/jobs.tspackages/playout-gateway/package.jsonpackages/playout-gateway/src/coreHandler.tspackages/playout-gateway/src/tsrHandler.tspackages/shared-lib/package.jsonpackages/shared-lib/src/peripheralDevice/methodsAPI.tspackages/shared-lib/src/pubsub/peripheralDevice.ts
✅ Files skipped from review due to trivial changes (1)
- packages/blueprints-integration/src/context/index.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/shared-lib/package.json
- packages/playout-gateway/package.json
- packages/playout-gateway/src/coreHandler.ts
- packages/job-worker/src/workers/studio/jobs.ts
- packages/corelib/src/worker/studio.ts
- meteor/server/worker/worker.ts
- packages/blueprints-integration/src/api/showStyle.ts
- packages/shared-lib/src/peripheralDevice/methodsAPI.ts
- packages/playout-gateway/src/tsrHandler.ts
- meteor/server/api/peripheralDevice.ts
- meteor/server/publications/externalEventSubscriptions.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the BBC
Type of Contribution
This is a: Feature
New Behavior
RFC: #1670
Allow TSR devices to report events when external state changes occur. This triggers a new blueprints method to allow for custom handling of these state changes.
Events are batched to avoid flooding the bluerpints/job-worker with a large backlog of events to handle
Testing
Affected areas
Time Frame
Other Information
Status